service-automation: populate the contained-failure visibility contract — FlowRunSummary.failed fold, loop iteration through try_catch, $error.iteration / $error.item, failed= on the summary line - #15609
Conversation
…ontract (#14456) `loop { body: [ try_catch { try, catch } ] }` contained a per-iteration failure but reported nothing about it: no run-level count, no iteration on the failing step, no row identity on `$error`. - `summarizeRun` folds `failed = Sigma nodes[].failures` over the array it publishes, so the run-level count cannot disagree with the breakdown. - `formatRunSummaryLine` prints `failed=N` whenever present, `failed=0` included; absent (an older row) prints nothing. Absent is "not tracked", never zero — no migration, no default. - `try_catch` forwards the enclosing loop's iteration into `runRegion`'s grouping for both regions, leaving the tagger and `parallel` untouched. - `$error` is the declared `TryCatchErrorValue`, gaining `iteration` and `item` inside a loop body and binding neither outside one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…ory pins (#14456) `check:engine-double-contract` counts unguarded doubles PER FILE against a shrink-only baseline. Three copies of the same four-member fake would have been two new ledger rows for one fact; one shared factory keeps the file's population at the 5 the baseline already records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
… `code` widening Refs #14456 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift Check9 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 026f57374ae4e1cf1c75d8961b66efecbd00489c && git checkout 026f57374ae4e1cf1c75d8961b66efecbd00489c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ba426b0f091b8d5901acf10bb8e649b514349f49 cb2bc181bee85fd78f87498695c7c3d523072d2e && git checkout -B drift-repro ba426b0f091b8d5901acf10bb8e649b514349f49 && git merge --no-ff cb2bc181bee85fd78f87498695c7c3d523072d2e
node scripts/docs-audit/affected-docs.mjs --json ba426b0f091b8d5901acf10bb8e649b514349f49 |
Contract review (Clause-②) — PR #15609 @
|
| mutation (injected=1 on disk, anchor=0 after) | pin turned red |
|---|---|
M1 failed=${summary.failed ?? 0} |
prints NOTHING for a summary recorded before the count existed |
M2 iteration: loopFrame?.iteration, item: loopFrame?.item |
binds NEITHER outside a loop and the subflow-leak pin |
M3 currentLoopIteration → return frame; |
does not leak a parent run's row identity into a subflow child |
M4 tagger writes iteration onto already-tagged steps |
leaves 'parallel' branch tagging exactly as it was |
Each file restored with git checkout HEAD -- <abs path>; git hash-object equals the HEAD blob for all four (9917dd08…, 04359f33…, a10242cb…, d765ab2e…). |
Anything new
N1 — defect, this PR (docs): content/docs/automation/flows.mdx:875 prints the run summary line as literal log output: …selected=30 acted=0 skipped=30 gate=check_stalled->send_nudge:30. After this PR that run prints …skipped=30 failed=0 gate=… — this PR updated its own pin to exactly that (run-summary.test.ts:150-156) and left the doc's copy of the same line stale, directly above the table row that documents failed and says "Absent on a run that did not track it". content/docs/automation/ is a hand-written tree (AGENTS.md doc guardrails: only references/ and releases/ are off-limits in a code PR), and the docs-drift bot on this PR says itself it cannot see this page.
N2 — spec-lane inconsistency, not this PR's code: per item 2, failed=0 on a parent that delegates rows to a subflow reads "nothing failed" (the changeset's words) while the children lost rows. The two spec paragraphs conflict for that case; the resolution (a failure slot on ExecutionStepMetrics that subflow/map roll up, or an explicit carve-out on failed) is a domain:spec decision. Third in the series after #14954 / #15606.
N3 — pre-existing engine defect, out of this PR's surface (file it): a map node inside a loop body runs its collection once, on the first iteration only. map-node.ts keeps ${node.id}.$mapState in the shared scope and never deletes it after "All items done" (variables.set(stateKey, state) ~line 212; zero variables.delete(stateKey) hits, control: the sibling $mapItemDone is deleted at lines 132-133), so iterations 2..n see started === collection.length and run nothing while the step reports success. Measured: 5 iterations × 2 items → 2 child runs, map step success on all five, run completed, failed=0. Silent partial work is the class #14456 exists to expose, and this one is invisible even to the new counter.
N4 — observation, #14414's: under a parallel branch inside a loop, the try/catch region's steps now carry the loop index while their container's own step carries the branch index; regionKind disambiguates. Unchanged writes by parallel, but it is the overload #14414 owns.
N5 — observation: $error.item is collection[i] captured at publish time; if the try region reassigns the iterator variable before failing, item still names the original row (measured: [2,'c3'] after an assignment overwrote currentCase). Arguably the better answer; the spec text says "the iteratorVariable value" — a one-word clarification at most.
Changes required (no code)
content/docs/automation/flows.mdx:875— bring the literal line to what the run now prints:… skipped=30 failed=0 gate=check_stalled->send_nudge:30, matchingrun-summary.test.ts:150-156.- Narrow the "
failed=0is the reading 'nothing failed'" sentence (changeset,run-summary.ts:197-205comment, and thefailedrow atflows.mdx) to what item 2 measured: no node execution of this run failed — asubflowchild's contained failures stay on the child's row. Cite the N2 issue. - File N2 (
domain:spec) and N3 (domain:services,map-in-loop) as issues and reference them from the PR body's "Out of scope" list, alongside spec:TryCatchErrorValueSchemasilently strips thecodekey the engine now binds — the declared "ONE shape" and the runtime shape have diverged (#14419 follow-up) #14954 / [finding] service-automation:FlowRunSummary.failedhas no queryable column onsys_automation_run, so "which runs lost rows?" is asummary_jsonscan #15606.
Generated by Claude Code
…ed=0` claims (#14456) Contract review, three docs-only changes; no engine change. - `flows.mdx` printed the summary line as literal log output and was left stale by this PR's own token: the run it shows now prints `skipped=30 failed=0 gate=…`, and the line sits directly above the table row documenting `failed`. - Narrowed the reading of `failed=0` in three places — the doc's `failed` row, `formatRunSummaryLine`'s comment and the changeset — from "nothing failed" to what was measured: no node execution OF THIS RUN failed. A `subflow` child's contained failures stay on the child's summary rather than rolling up the way `acted` does. Cites #15617, where the declaration's two paragraphs are being reconciled. `content/docs/releases/v17.mdx` carries the same line and is deliberately NOT touched: release notes are a record of what that release printed, and are never edited from a code PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
Contract review (Clause-②) — round 2 — PR #15609 @
|
Fixes #14456
The engine half of #13681's visibility rider.
loop { body: [ try_catch { try, catch } ] }is the ruled containment spelling for a per-iteration failure that must not end the sweep (maintainer 2026-08-31, director batch #18, verbatim 「其他同意」; branch B selected by measurement — there is deliberately noloop.config.onIterationErrorkey). Containment already worked. What did not exist was any way to see what it had contained.The contract this populates was declared by PR #14452 and is already on
main:FlowRunSummary.failed,TryCatchErrorValueSchema, and theiteration/regionKindprose onExecutionStepLogSchema. ⛔ Nopackages/specfile is touched by this PR — this fills declared slots, it does not open new ones.The measurement this reproduces
#13681 comment 5478851960, on the real
AutomationEngine: five rows, the third ownerless,notifyfailing on it. Contained correctly, and reported like this:Five iterations ran, one row was lost, and nothing in that line, in the run row, or on
$errorsaid so. The failing step carried no iteration index;$errorwas{ nodeId: 'guard', message: "…" }and named the row only because the message happened to echo the template. A sweep that lost two rows out of five was indistinguishable from one that lost none.That fixture is now a committed test:
packages/services/service-automation/src/builtin/contained-failure-visibility.test.ts.What changed
1 ·
FlowRunSummary.failed—summarizeRunfoldsfailed = Σ nodes[].failures. Computed in the loop that already walksnodes.values()to settle each node's status, rather than as a second accumulator in the step loop: two counters over one fact drift, one addition cannot, and the field is DECLARED as a fold ("stated as a fold so the run-level count can never disagree with the per-node breakdown"). The test asserts the identity directly, not just the number.2 · Per node — no change, as the card specifies. Verified rather than assumed:
run-summary.tsalready doesif (step.status === 'failure') node.failures += 1;per step. Nothing was added.3 · Iteration through
try_catch→runRegion. The card left this choice to the implementer, fenced on leavingparalleluntouched. Chosen: forward the loop'siterationat the try/catch call site. Both options and why this one:iterationon already-tagged steps.runRegion's tagger is shared by every container.parallelcalls it withiteration:set to the BRANCH INDEX (its pre-existing overload, Aparallelbranch inside aloopbody overloads the step record'siterationwith the branch index — the enclosing loop iteration is lost, so a branch step cannot be attributed to its row #14414's subject). Under that change atry/catchregion nested inside aparallelbranch would start receiving the BRANCH index oniterationfromparallel's own tagger — a change to whatparallelwrites, in exactly the territory the fence rules out.try-catch-node.tsaddsiterationto the grouping it handsrunRegionfor both regions.runRegionandparallel-node.tsare byte-unchanged, and aparallelbranch step still carries its branch index exactly as before. There is a pin for that:leaves 'parallel' branch tagging exactly as it was — the #14414 fence, which passes identically with and without this implementation (see the ablation below — it is one of the four that must NOT move).The forwarded value comes from a new module,
builtin/loop-frame.ts: the loop publishes{ iteration, item, scope }for the duration of each body region, and a nested container reads it.AsyncLocalStorage(the primitive@objectstack/objectqlalready uses for its ambient transaction) rather than a field on the engine, because twoparallelbranches run their bodies concurrently over one engine and a stored "current iteration" would be read by whichever continuation resumed next. The frame also names the variable scope it belongs to and a reader must present a matching one — a region runs in the enclosing scope, asubflow/mapchild run gets a freshMap, so a parent's row identity cannot leak into a child run's$error. That is pinned too.4 ·
$errorbinds the row. The value bound toerrorVariableis the declaredTryCatchErrorValue, gainingiterationand the loop's currentiteminside a loop body and binding neither outside one. The test asserts the bound valuesafeParses underTryCatchErrorValueSchema, so the shape is the contract's and not this executor's invention.code(#14419), whichTryCatchErrorValueSchemadoes not declare. Dropping it would regress a catch region's ability to branch on{$error.code}, so it is spelled as an explicitTryCatchErrorValue & { code?: string }widening with a comment — loud and removable — rather than silently dropped or silently smuggled. The divergence is already filed as #14954 against the spec lane and is not addressed here; that issue remains open.5 ·
failed=on the summary line.formatRunSummaryLineprints the token whenever the count is PRESENT,failed=0included. It sits betweenskipped=and the optionalunmeasured=/gate=tokens, so the always-present prefix stays greppable as one unit.Read
failed=0precisely, because it is narrower than it looks: no node execution OF THIS RUN failed. The fold isΣ nodes[].failuresover this run's own nodes, so asubflowchild that CONTAINED failures of its own reports them on the child's summary and the parent still printsfailed=0— unlikeacted, which does roll a child's totals up. That asymmetry is in the declaration, not in this implementation, and is filed as #15617. The narrowing is written into all three places a reader meets the claim: thefailedrow inflows.mdx,formatRunSummaryLine's comment, and the changeset.failedabsent means "not tracked" — it is NEVER defaulted to0.A run recorded before this change did not carry the count. Defaulting it to zero would tell an operator "nothing failed" about a run nobody measured — the same reasoning
unmeasuredcarries three lines above it in the schema, and the reason both are.optional()there. So:failedabsent — pinned;failed=token at all, which is a different reading fromfailed=0;summarizeRunalways emits the count,0included, so the absent case belongs only to stored rows.Persistence
The count rides in
summary_jsonwith the rest of the fold, and — the branch worth checking — it is kept whenserializeSummaryBoundeddropsnodes/gatespast the 16 KiB cap. That branch drops exactly thenodes[].failuresthis folds, so a compacted summary would otherwise go from "some rows failed" to silence. Pinned with a 600-node fixture.It gets no column of its own on
sys_automation_run, deliberately: the four existing count columns are there because the broken-sweep FILTER queries them, and this is a stored-surface change on an ADR-0103 engine-owned object. Whether it should have one is filed separately as #15606 (a finding, not addressed here — that issue remains open).Also out of scope by construction: the fold is exactly
Σ nodes[].failures, so asubflowchild's contained failures stay in the child's own summary rather than rolling up the wayacteddoes. That follows from the declared fold, so it is a property, not a gap.Verification
Base
900334a56, final commitcb2bc181b(round 2; the engine work is unchanged since56eeff22e— round 2 is prose and one.mdx).pnpm --filter @objectstack/service-automation testpnpm --filter @objectstack/service-automation typechecktsc --noEmit+check:test-typecheck, 0 errors)pnpm lint(eslint . --no-inline-config, whole repo)scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack)cb2bc181b(was 67/60/0 before the.mdxedit pulled in the docs family)node scripts/check-adr-0087-registration.mjs --base origin/main --head cb2bc181b--self-testrequire('dist/index.js'))loop-frame.tsadds anode:async_hooksimportThe 7 non-zero gate exits are all NOT MEASURED, not red, and each says so itself:
check-partof-closing-keyword/check-single-claim-pathsprintNOT WIRED — … judged nothing;check-test-completeness,check-half-statesandcheck:dual-build-cjs-loadsprintPREREQUISITE NOT MET(a saved turbo log, a repo-scoped GitHub read this container is refused, and a fullpnpm buildrespectively);check:published-readme-exportswants the same full build;pr-labels.mjsprinted its usage. None is a verdict about this diff. Exit codes were captured by redirect, never across a pipe.check:engine-double-contractdid red once, honestly: the new run-history pins added two more unguarded engine doubles torun-summary.test.ts, over a shrink-only baseline of 5. Fixed the way the gate itself advises — by reusing the double the file already has (one sharedrecordingRunStore()factory for the threerecordTerminalpins) rather than raising the ledger. The baseline is untouched and the file's population is still 5.Review round 2 — docs only, no engine change
Contract review at tier returned CHANGES REQUIRED with three prose items; the engine work passed unchanged.
content/docs/automation/flows.mdx:875was stale. It prints the run summary line as literal log output, directly above the table row that documentsfailed, and this PR's own token had been added to the test pin (run-summary.test.ts) while the doc's copy of the same line was left alone. It now reads… skipped=30 failed=0 gate=….content/docs/releases/v17.mdx:1607carries the same line and is deliberately not touched: release notes are a record of what that release printed, and are never edited from a code PR.failed=0reading is narrowed to what was measured, in the doc row, theformatRunSummaryLinecomment and the changeset — see item 5 above.Re-verified on
cb2bc181b: derived gate family 81 run · 74 exit 0 · 0 red (the.mdxedit pulls in 14 docs gates the earlier rounds never touched —check:doc-authoring,check:doc-anchors,check:doc-frontmatter,check:doc-route-spelling,check:docs-section-name,check:section-landing-index,check:docs-single-h1,check:docs-redirects,check:docs-audit-scope,check:corpus-claim-drift,check:published-readme-links,check:role-word,check:vendor-version-stamps,check:skill-identifier-liveness— all exit 0, as do bothscripts/docs-audit/members and all four changeset gates); package suite 107 files / 1290 tests passed; typecheck green;pnpm lint(whole repo) exit 0; ADR-0087 real invocation exit 0. The same 7 NOT-MEASURED exits as before, unchanged.Ablation
Reverted the three implementation files to the base commit and deleted
loop-frame.ts, with the mutation confirmed on disk by grepping both the injected and the removed text (failed += node.failures→ 0,if (summary.unmeasured) parts.pushback at 1,loopFrame→ 0,runInLoopIteration→ 0,loop-frame.tsABSENT):No rebuild step is involved and none is claimed: every mutated file is imported by the tests through a relative specifier inside the same package, so vitest reads the source, never a stale
dist/. The 12 reds prove it.Four of the new pins deliberately stayed green through the ablation, and that is the correct reading rather than a weakness — they pin what must NOT change:
parallelbranch tagging, no binding outside a loop, no leak into a subflow child, and an older summary printing nofailed=.Restored under a trap with absolute paths, then proven —
git hash-objecton each of the four files equals its HEAD blob, andgit diff HEADis empty.Premise check
Each of the PM's five measurements was re-run on this base rather than inherited:
FlowRunSummarySchemaalready declaresfailed, documented as the fold, with the absent-is-not-zero convention written out.packages/specis untouched.failuresincrement already exists; nothing was added for item 2.TryCatchErrorValueSchemaimports cleanly from@objectstack/spec/automation.summarizeRun/formatRunSummaryLinelive inrun-summary.ts(called fromengine.tsand re-exported fromindex.ts). No line number from the card or its refs was built on.try-catch-node.tswas clean at the base.Out of scope, not widened into
⛔ #14414 (
parallel-in-loopiteration attribution) · ⛔ #13803 (acted: 0after a mid-loop death) · ⛔ #14394 (the lint and docs limbs) · ⛔packages/spec.Four issues this work touches and does not address — each open, none of them addressed by this diff:
TryCatchErrorValueSchemasilently strips thecodekey the engine now binds — the declared "ONE shape" and the runtime shape have diverged (#14419 follow-up) #14954 —TryCatchErrorValueSchemadoes not declare thecodekey the engine binds. Not worked around by droppingcode; spelled as the explicit widening described above.FlowRunSummary.failedhas no queryable column onsys_automation_run, so "which runs lost rows?" is asummary_jsonscan #15606 —FlowRunSummary.failedhas no queryable column onsys_automation_run.FlowRunSummary's two paragraphs disagree for a subflow parent —failedis declared a node fold, while the summary is declared to answer "what did this run cause" and roll a child's totals up #15617 — the declaration disagrees with itself for asubflowparent:execution.zod.tsdeclares the summary as answering "what did this run cause", rolling a child's totals up, and declaresfailedas the node fold. This PR implements the fold, which is what the field's own docblock says; reconciling the two paragraphs is the spec lane's call, not this card's.mapnode inside aloopbody runs its collection ONCE — iterations 2..n do nothing, reportsuccess, and the run completes green #15616 — a pre-existing engine defect found past this diff by the review: amapnode inside aloopbody runs its collection ONCE, because${node.id}.$mapStateis left in the shared scope while the sibling$mapItemDoneis deleted. Iterations 2..n do nothing, reportsuccess, and the run completes green. Not fixed here. Worth stating plainly: that is silent partial work — the exact class this card exists to expose — and the new counter does not see it, because nothing failed.Clause-② applies. The governance-tier contract review has run and returned CHANGES REQUIRED on three prose items, all addressed above; the PR stays a draft and landing is the PM's. The public type surface does not widen — spec already declares every field — but what a shipped surface REPORTS does: a consumer reading
summary.failedstarts getting a number where it gotundefined, andformatRunSummaryLineemits a token on a line that previously carried none.skip-changesetdoes not apply:@objectstack/service-automationpublishes, and a changeset is included.